From dfc3e3df90a3293ef3c12935235305f7af0803ff Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 16 Sep 2013 12:31:40 +0200 Subject: [PATCH] Correctly update wl_notificationtimestamp when viewing old revisions == Prelude == wl_notificationtimestamp controls sending the user e-mail notifications about changes to pages, as well as showing the "updated since last visit" markers on history pages, recent changes and watchlist. == The bug == Previously, on every view of a page, the notification timestamp was cleared, regardless of whether the user as actually viewing the latest revision. When viewing a diff, however, the timestamp was cleared only if one of the revisions being compared was the latest one of its page. The same behavior applied to talk page message indicators (which are actually stored sepately to cater to anonymous users). This was inconsistent and surprising when one was attempting to, say, go through the 50 new posts to a discussion page in a peacemeal fashion. == The fix == If the revision being viewed is the latest (or can't be determined), the timestamp is cleared as previously, as this is necessary to reenable e-mail notifications for given user and page. If the revision isn't the latest, the timestamp is updated to revision's timestamp plus one second. This uses up to two simple (selectField) indexed queries per page view, only fired when we do not already know we're looking at the latest version. Talk page indicator is updated to point at the next revision after the one being viewed, or cleared if viewing the latest revision. The UserClearNewTalkNotification hook gained $oldid as the second argument (a backwards-compatible change). In Skin, we no longer ignore the indicator being present if we're viewing the talk page, as it might still be valid. == The bonus == Comments and formatting was updated in a few places, including tables.sql and Wiki.php. The following functions gained a second, optional $oldid parameter (holy indirection, Batman!): * WikiPage#doViewUpdates() * User#clearNotification() * WatchedItem#resetNotificationTimestamp() DifferenceEngine gained a public method mapDiffPrevNext() used to parse the ids from URL parameters like oldid=12345&diff=prev, factored out of loadRevisionIds(). A bug where the NewDifferenceEngine hook would not be called in some cases, dating back to its introduction in r45518, was fixed in the process. Bug: 41759 Change-Id: I4144ba1987b8d7a7e8b24f4f067eedac2ae44459 --- RELEASE-NOTES-1.23 | 4 ++ docs/hooks.txt | 1 + includes/Article.php | 12 ++-- includes/ImagePage.php | 4 +- includes/Skin.php | 99 +++++++++++++++--------------- includes/User.php | 38 ++++++++---- includes/WatchedItem.php | 45 +++++++++++++- includes/Wiki.php | 1 + includes/WikiPage.php | 7 ++- includes/diff/DifferenceEngine.php | 53 ++++++++++------ maintenance/tables.sql | 5 +- 11 files changed, 171 insertions(+), 98 deletions(-) diff --git a/RELEASE-NOTES-1.23 b/RELEASE-NOTES-1.23 index 372170a768..ec7b898f6f 100644 --- a/RELEASE-NOTES-1.23 +++ b/RELEASE-NOTES-1.23 @@ -13,6 +13,10 @@ production. === New features in 1.23 === === Bug fixes in 1.23 === +* (bug 41759) The "updated since last visit" markers (on history pages, recent + changes and watchlist) and the talk page message indicator are now correctly + updated when the user is viewing old revisions of pages, instead of always + acting as if the latest revision was being viewed. === API changes in 1.23 === diff --git a/docs/hooks.txt b/docs/hooks.txt index 5aaf596103..2671dd8dae 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -2579,6 +2579,7 @@ $user: User (object) whose permission is being checked 'UserClearNewTalkNotification': Called when clearing the "You have new messages!" message, return false to not delete it. $user: User (object) that will clear the message +$oldid: ID of the talk page revision being viewed (0 means the most recent one) 'UserComparePasswords': Called when checking passwords, return false to override the default password checks. diff --git a/includes/Article.php b/includes/Article.php index 0b18221a0a..928fda0a98 100644 --- a/includes/Article.php +++ b/includes/Article.php @@ -586,7 +586,7 @@ class Article implements Page { wfDebug( __METHOD__ . ": done file cache\n" ); # tell wgOut that output is taken care of $outputPage->disable(); - $this->mPage->doViewUpdates( $user ); + $this->mPage->doViewUpdates( $user, $oldid ); wfProfileOut( __METHOD__ ); return; @@ -765,7 +765,7 @@ class Article implements Page { $outputPage->setFollowPolicy( $policy['follow'] ); $this->showViewFooter(); - $this->mPage->doViewUpdates( $user ); + $this->mPage->doViewUpdates( $user, $oldid ); $outputPage->addModules( 'mediawiki.action.view.postEdit' ); @@ -815,10 +815,10 @@ class Article implements Page { $this->mRevIdFetched = $de->mNewid; $de->showDiffPage( $diffOnly ); - if ( $diff == 0 || $diff == $this->mPage->getLatest() ) { - # Run view updates for current revision only - $this->mPage->doViewUpdates( $user ); - } + // Run view updates for the newer revision being diffed (and shown below the diff if not $diffOnly) + list( $old, $new ) = $de->mapDiffPrevNext( $oldid, $diff ); + // New can be false, convert it to 0 - this conveniently means the latest revision + $this->mPage->doViewUpdates( $user, (int)$new ); } /** diff --git a/includes/ImagePage.php b/includes/ImagePage.php index 515f146e56..cf05ee2585 100644 --- a/includes/ImagePage.php +++ b/includes/ImagePage.php @@ -128,7 +128,7 @@ class ImagePage extends Article { $out->setPageTitle( $this->getTitle()->getPrefixedText() ); $out->addHTML( $this->viewRedirect( Title::makeTitle( NS_FILE, $this->mPage->getFile()->getName() ), /* $appendSubtitle */ true, /* $forceKnown */ true ) ); - $this->mPage->doViewUpdates( $this->getContext()->getUser() ); + $this->mPage->doViewUpdates( $this->getContext()->getUser(), $this->getOldID() ); return; } } @@ -165,7 +165,7 @@ class ImagePage extends Article { # Just need to set the right headers $out->setArticleFlag( true ); $out->setPageTitle( $this->getTitle()->getPrefixedText() ); - $this->mPage->doViewUpdates( $this->getContext()->getUser() ); + $this->mPage->doViewUpdates( $this->getContext()->getUser(), $this->getOldID() ); } # Show shared description, if needed diff --git a/includes/Skin.php b/includes/Skin.php index 5801806c9a..170e96fc19 100644 --- a/includes/Skin.php +++ b/includes/Skin.php @@ -1379,61 +1379,58 @@ abstract class Skin extends ContextSource { if ( count( $newtalks ) == 1 && $newtalks[0]['wiki'] === wfWikiID() ) { $uTalkTitle = $user->getTalkPage(); - - if ( !$uTalkTitle->equals( $out->getTitle() ) ) { - $lastSeenRev = isset( $newtalks[0]['rev'] ) ? $newtalks[0]['rev'] : null; - $nofAuthors = 0; - if ( $lastSeenRev !== null ) { - $plural = true; // Default if we have a last seen revision: if unknown, use plural - $latestRev = Revision::newFromTitle( $uTalkTitle, false, Revision::READ_NORMAL ); - if ( $latestRev !== null ) { - // Singular if only 1 unseen revision, plural if several unseen revisions. - $plural = $latestRev->getParentId() !== $lastSeenRev->getId(); - $nofAuthors = $uTalkTitle->countAuthorsBetween( - $lastSeenRev, $latestRev, 10, 'include_new' ); - } - } else { - // Singular if no revision -> diff link will show latest change only in any case - $plural = false; + $lastSeenRev = isset( $newtalks[0]['rev'] ) ? $newtalks[0]['rev'] : null; + $nofAuthors = 0; + if ( $lastSeenRev !== null ) { + $plural = true; // Default if we have a last seen revision: if unknown, use plural + $latestRev = Revision::newFromTitle( $uTalkTitle, false, Revision::READ_NORMAL ); + if ( $latestRev !== null ) { + // Singular if only 1 unseen revision, plural if several unseen revisions. + $plural = $latestRev->getParentId() !== $lastSeenRev->getId(); + $nofAuthors = $uTalkTitle->countAuthorsBetween( + $lastSeenRev, $latestRev, 10, 'include_new' ); } - $plural = $plural ? 2 : 1; - // 2 signifies "more than one revision". We don't know how many, and even if we did, - // the number of revisions or authors is not necessarily the same as the number of - // "messages". - $newMessagesLink = Linker::linkKnown( - $uTalkTitle, - $this->msg( 'newmessageslinkplural' )->params( $plural )->escaped(), - array(), - array( 'redirect' => 'no' ) - ); + } else { + // Singular if no revision -> diff link will show latest change only in any case + $plural = false; + } + $plural = $plural ? 2 : 1; + // 2 signifies "more than one revision". We don't know how many, and even if we did, + // the number of revisions or authors is not necessarily the same as the number of + // "messages". + $newMessagesLink = Linker::linkKnown( + $uTalkTitle, + $this->msg( 'newmessageslinkplural' )->params( $plural )->escaped(), + array(), + array( 'redirect' => 'no' ) + ); - $newMessagesDiffLink = Linker::linkKnown( - $uTalkTitle, - $this->msg( 'newmessagesdifflinkplural' )->params( $plural )->escaped(), - array(), - $lastSeenRev !== null - ? array( 'oldid' => $lastSeenRev->getId(), 'diff' => 'cur' ) - : array( 'diff' => 'cur' ) - ); + $newMessagesDiffLink = Linker::linkKnown( + $uTalkTitle, + $this->msg( 'newmessagesdifflinkplural' )->params( $plural )->escaped(), + array(), + $lastSeenRev !== null + ? array( 'oldid' => $lastSeenRev->getId(), 'diff' => 'cur' ) + : array( 'diff' => 'cur' ) + ); - if ( $nofAuthors >= 1 && $nofAuthors <= 10 ) { - $newMessagesAlert = $this->msg( - 'youhavenewmessagesfromusers', - $newMessagesLink, - $newMessagesDiffLink - )->numParams( $nofAuthors ); - } else { - // $nofAuthors === 11 signifies "11 or more" ("more than 10") - $newMessagesAlert = $this->msg( - $nofAuthors > 10 ? 'youhavenewmessagesmanyusers' : 'youhavenewmessages', - $newMessagesLink, - $newMessagesDiffLink - ); - } - $newMessagesAlert = $newMessagesAlert->text(); - # Disable Squid cache - $out->setSquidMaxage( 0 ); + if ( $nofAuthors >= 1 && $nofAuthors <= 10 ) { + $newMessagesAlert = $this->msg( + 'youhavenewmessagesfromusers', + $newMessagesLink, + $newMessagesDiffLink + )->numParams( $nofAuthors ); + } else { + // $nofAuthors === 11 signifies "11 or more" ("more than 10") + $newMessagesAlert = $this->msg( + $nofAuthors > 10 ? 'youhavenewmessagesmanyusers' : 'youhavenewmessages', + $newMessagesLink, + $newMessagesDiffLink + ); } + $newMessagesAlert = $newMessagesAlert->text(); + # Disable Squid cache + $out->setSquidMaxage( 0 ); } elseif ( count( $newtalks ) ) { $sep = $this->msg( 'newtalkseparator' )->escaped(); $msgs = array(); diff --git a/includes/User.php b/includes/User.php index 12912e1c0a..c86b966ffe 100644 --- a/includes/User.php +++ b/includes/User.php @@ -3021,8 +3021,9 @@ class User { * the next change of the page if it's watched etc. * @note If the user doesn't have 'editmywatchlist', this will do nothing. * @param $title Title of the article to look at + * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed. */ - public function clearNotification( &$title ) { + public function clearNotification( &$title, $oldid = 0 ) { global $wgUseEnotif, $wgShowUpdatedMarker; // Do nothing if the database is locked to writes @@ -3035,12 +3036,25 @@ class User { return; } - if ( $title->getNamespace() == NS_USER_TALK && - $title->getText() == $this->getName() ) { - if ( !wfRunHooks( 'UserClearNewTalkNotification', array( &$this ) ) ) { + // If we're working on user's talk page, we should update the talk page message indicator + if ( $title->getNamespace() == NS_USER_TALK && $title->getText() == $this->getName() ) { + if ( !wfRunHooks( 'UserClearNewTalkNotification', array( &$this, $oldid ) ) ) { return; } - $this->setNewtalk( false ); + + $nextid = $oldid ? $title->getNextRevisionID( $oldid ) : null; + + if ( !$oldid || !$nextid ) { + // If we're looking at the latest revision, we should definitely clear it + $this->setNewtalk( false ); + } else { + // Otherwise we should update its revision, if it's present + if ( $this->getNewtalk() ) { + // Naturally the other one won't clear by itself + $this->setNewtalk( false ); + $this->setNewtalk( true, Revision::newFromId( $nextid ) ); + } + } } if ( !$wgUseEnotif && !$wgShowUpdatedMarker ) { @@ -3063,7 +3077,7 @@ class User { $force = 'force'; } - $this->getWatchedItem( $title )->resetNotificationTimestamp( $force ); + $this->getWatchedItem( $title )->resetNotificationTimestamp( $force, $oldid ); } /** @@ -3091,14 +3105,12 @@ class User { if ( $id != 0 ) { $dbw = wfGetDB( DB_MASTER ); $dbw->update( 'watchlist', - array( /* SET */ - 'wl_notificationtimestamp' => null - ), array( /* WHERE */ - 'wl_user' => $id - ), __METHOD__ + array( /* SET */ 'wl_notificationtimestamp' => null ), + array( /* WHERE */ 'wl_user' => $id ), + __METHOD__ ); - # We also need to clear here the "you have new message" notification for the own user_talk page - # This is cleared one page view later in Article::viewUpdates(); + // We also need to clear here the "you have new message" notification for the own user_talk page; + // it's cleared one page view later in WikiPage::doViewUpdates(). } } diff --git a/includes/WatchedItem.php b/includes/WatchedItem.php index 1e07e7c795..d2fb468335 100644 --- a/includes/WatchedItem.php +++ b/includes/WatchedItem.php @@ -173,8 +173,9 @@ class WatchedItem { * * @param $force Whether to force the write query to be executed even if the * page is not watched or the notification timestamp is already NULL. + * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed. */ - public function resetNotificationTimestamp( $force = '' ) { + public function resetNotificationTimestamp( $force = '', $oldid = 0 ) { // Only loggedin user can have a watchlist if ( wfReadOnly() || $this->mUser->isAnon() || !$this->isAllowed( 'editmywatchlist' ) ) { return; @@ -187,10 +188,50 @@ class WatchedItem { } } + $title = $this->getTitle(); + if ( !$oldid ) { + // No oldid given, assuming latest revision; clear the timestamp. + $notificationTimestamp = null; + } elseif ( !$title->getNextRevisionID( $oldid ) ) { + // Oldid given and is the latest revision for this title; clear the timestamp. + $notificationTimestamp = null; + } else { + // See if the version marked as read is more recent than the one we're viewing. + // Call load() if it wasn't called before due to $force. + $this->load(); + + if ( $this->timestamp === null ) { + // This can only happen if $force is enabled. + $notificationTimestamp = null; + } else { + // Oldid given and isn't the latest; update the timestamp. + // This will result in no further notification emails being sent! + $dbr = wfGetDB( DB_SLAVE ); + $notificationTimestamp = $dbr->selectField( + 'revision', 'rev_timestamp', + array( 'rev_page' => $title->getArticleID(), 'rev_id' => $oldid ) + ); + // We need to go one second to the future because of various strict comparisons + // throughout the codebase + $ts = new MWTimestamp( $notificationTimestamp ); + $ts->timestamp->add( new DateInterval( 'PT1S' ) ); + $notificationTimestamp = $ts->getTimestamp( TS_MW ); + + if ( $notificationTimestamp < $this->timestamp ) { + if ( $force != 'force' ) { + return; + } else { + // This is a little silly… + $notificationTimestamp = $this->timestamp; + } + } + } + } + // If the page is watched by the user (or may be watched), update the timestamp on any // any matching rows $dbw = wfGetDB( DB_MASTER ); - $dbw->update( 'watchlist', array( 'wl_notificationtimestamp' => null ), + $dbw->update( 'watchlist', array( 'wl_notificationtimestamp' => $notificationTimestamp ), $this->dbCond(), __METHOD__ ); $this->timestamp = null; } diff --git a/includes/Wiki.php b/includes/Wiki.php index edfbba2d28..403ef110c6 100644 --- a/includes/Wiki.php +++ b/includes/Wiki.php @@ -588,6 +588,7 @@ class MediaWiki { $cache->loadFromFileCache( $this->context ); } // Do any stats increment/watchlist stuff + // Assume we're viewing the latest revision (this should always be the case with file cache) $this->context->getWikiPage()->doViewUpdates( $this->context->getUser() ); // Tell OutputPage that output is taken care of $this->context->getOutput()->disable(); diff --git a/includes/WikiPage.php b/includes/WikiPage.php index 7c3dc9374c..6d2d80c6bf 100644 --- a/includes/WikiPage.php +++ b/includes/WikiPage.php @@ -1134,9 +1134,10 @@ class WikiPage implements Page, IDBAccessObject { /** * Do standard deferred updates after page view - * @param $user User The relevant user + * @param User $user The relevant user + * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed. */ - public function doViewUpdates( User $user ) { + public function doViewUpdates( User $user, $oldid = 0 ) { global $wgDisableCounters; if ( wfReadOnly() ) { return; @@ -1149,7 +1150,7 @@ class WikiPage implements Page, IDBAccessObject { } // Update newtalk / watchlist notification status - $user->clearNotification( $this->mTitle ); + $user->clearNotification( $this->mTitle, $oldid ); } /** diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php index e436f58d30..ea741641b4 100644 --- a/includes/diff/DifferenceEngine.php +++ b/includes/diff/DifferenceEngine.php @@ -1041,6 +1041,33 @@ class DifferenceEngine extends ContextSource { $this->mDiffLang = wfGetLangObj( $lang ); } + /** + * Maps a revision pair definition as accepted by DifferenceEngine constructor + * to a pair of actual integers representing revision ids. + * + * @param int $old Revision id, e.g. from URL parameter 'oldid' + * @param int|string $new Revision id or strings 'next' or 'prev', e.g. from URL parameter 'diff' + * @return array Array of two revision ids, older first, later second. + * Zero signifies invalid argument passed. + * false signifies that there is no previous/next revision ($old is the oldest/newest one). + */ + public function mapDiffPrevNext( $old, $new ) { + if ( $new === 'prev' ) { + // Show diff between revision $old and the previous one. Get previous one from DB. + $newid = intval( $old ); + $oldid = $this->getTitle()->getPreviousRevisionID( $newid ); + } elseif ( $new === 'next' ) { + // Show diff between revision $old and the next one. Get next one from DB. + $oldid = intval( $old ); + $newid = $this->getTitle()->getNextRevisionID( $oldid ); + } else { + $oldid = intval( $old ); + $newid = intval( $new ); + } + + return array( $oldid, $newid ); + } + /** * Load revision IDs */ @@ -1054,26 +1081,14 @@ class DifferenceEngine extends ContextSource { $old = $this->mOldid; $new = $this->mNewid; - if ( $new === 'prev' ) { - # Show diff between revision $old and the previous one. - # Get previous one from DB. - $this->mNewid = intval( $old ); - $this->mOldid = $this->getTitle()->getPreviousRevisionID( $this->mNewid ); - } elseif ( $new === 'next' ) { - # Show diff between revision $old and the next one. - # Get next one from DB. - $this->mOldid = intval( $old ); - $this->mNewid = $this->getTitle()->getNextRevisionID( $this->mOldid ); - if ( $this->mNewid === false ) { - # if no result, NewId points to the newest old revision. The only newer - # revision is cur, which is "0". - $this->mNewid = 0; - } - } else { - $this->mOldid = intval( $old ); - $this->mNewid = intval( $new ); - wfRunHooks( 'NewDifferenceEngine', array( $this->getTitle(), &$this->mOldid, &$this->mNewid, $old, $new ) ); + list( $this->mOldid, $this->mNewid ) = self::mapDiffPrevNext( $old, $new ); + if ( $new === 'next' && $this->mNewid === false ) { + # if no result, NewId points to the newest old revision. The only newer + # revision is cur, which is "0". + $this->mNewid = 0; } + + wfRunHooks( 'NewDifferenceEngine', array( $this->getTitle(), &$this->mOldid, &$this->mNewid, $old, $new ) ); } /** diff --git a/maintenance/tables.sql b/maintenance/tables.sql index de92ef53e9..61ffcf4982 100644 --- a/maintenance/tables.sql +++ b/maintenance/tables.sql @@ -1109,8 +1109,9 @@ CREATE TABLE /*_*/watchlist ( wl_namespace int NOT NULL default 0, wl_title varchar(255) binary NOT NULL default '', - -- Timestamp when user was last sent a notification e-mail; - -- cleared when the user visits the page. + -- Timestamp used to send notification e-mails and show "updated since last visit" markers on + -- history and recent changes / watchlist. Set to NULL when the user visits the latest revision + -- of the page, which means that they should be sent an e-mail on the next change. wl_notificationtimestamp varbinary(14) ) /*$wgDBTableOptions*/; -- 2.20.1